New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to configure the resolve-url-loader #603
Conversation
Error: resolve-url-loader: CSS error source-map information is not available at url() declaration (found orphan CR, try removeCR option) see: bholloway/resolve-url-loader#107
Hey @diegocardoso93
Do you know if it could have any drawbacks on setups that don't need it to be If that's the case it could be better to allow setting it easily (maybe using a new option in |
Hello @Lyrkan |
@Lyrkan I've just updated with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @diegocardoso93,
Yes, I think that's better, I only have a few remarks :)
Also, could you add some documentation here?
Lines 749 to 756 in 9a3cb43
* Supported options: | |
* * {boolean} resolveUrlLoader (default=true) | |
* Whether or not to use the resolve-url-loader. | |
* Setting to false can increase performance in some | |
* cases, especially when using bootstrap_sass. But, | |
* when disabled, all url()'s are resolved relative | |
* to the original entry file... not whatever file | |
* the url() appears in. |
lib/loaders/sass.js
Outdated
@@ -31,7 +31,8 @@ module.exports = { | |||
sassLoaders.push({ | |||
loader: 'resolve-url-loader', | |||
options: { | |||
sourceMap: webpackConfig.useSourceMaps | |||
sourceMap: webpackConfig.useSourceMaps, | |||
...webpackConfig.sassOptions.resolveUrlLoaderOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the spread operator is allowed in this context with the version of ESLint we are currently using (which is why the "lint" job fails on Travis).
So two choices there:
- update ESLint (but maybe that's a chore for another PR)
- use
Object.assign()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the issue is not the eslint version, but the node.js version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter is executed using Node 10 which should supports this.
It should even be ok under Node 8 since that job is green: https://travis-ci.org/symfony/webpack-encore/jobs/560124277
$ nvm use v8.15.1
Now using node v8.15.1 (npm v6.4.1)
$ node
> const foo = { foo: 1, bar: 2 };
undefined
> const bar = { ...foo, bar: 3 };
undefined
> bar
{ foo: 1, bar: 3 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that could be solved without updating eslint though, by changing the following option of our .eslintrc.js
to 2018
:
Line 9 in 9a3cb43
"parserOptions": { "ecmaVersion": 2017 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you were partially right @stof, using ecmaVersion: 2018
is triggering other errors:
99:22 error Spread properties are not supported yet on Node 8.0.0 node/no-unsupported-features
99:22 error Rest/spread properties are not supported until Node.js 8.3.0. The configured version range is '8.* || >= 10.*' node/no-unsupported-features/es-syntax
We should probably change that range since most Webpack loaders are now requiring >= 8.9.0
(for instance: css-loader
, url-loader
, file-loader
, ...)
But as I said earlier, maybe it should be handled in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just updated the PR with changes requested.
Used Object.assign()
option for now.
Very sensible & clean addition. Thank you Diego! |
…3, Diego) This PR was merged into the master branch. Discussion ---------- Allow to configure the resolve-url-loader This PR solve the error below, occuring on some cases: ``` Error: resolve-url-loader: CSS error source-map information is not available at url() declaration (found orphan CR, try removeCR option) ``` See discussion: bholloway/resolve-url-loader#107 Commits ------- b7c7e66 Add documentation for resolveUrlLoaderOptions 4671016 Fix lint job 392d32b Merge branch 'master' of https://github.com/diegocardoso93/webpack-encore a52bb05 Add optional resolve-url-loader options 01e1da7 Add optional resolve-url-loader options 906ae9b Fix "found orphan CR, try removeCR option" 988e5d1 Update resolve-url-loader to v3.1.0
This PR solve the error below, occuring on some cases:
See discussion: bholloway/resolve-url-loader#107